Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rtl872x] hal: fix issue that uart rx dma may hang up. #2502

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

XuGuohui
Copy link
Member

Problem

When running the Serial2 stress test app as attached below, device may hang up over time.

Solution

  1. Implement recursive TX/RX lock
  2. Fix minor bug in ring buffer
  3. Do not suspend DMA unless necessary.

Steps to Test

Build and run the below test app.

Example App

#include "Particle.h"

SYSTEM_MODE(MANUAL);
SYSTEM_THREAD(ENABLED);

Serial1LogHandler log(115200, LOG_LEVEL_ALL, {
    {"system", LOG_LEVEL_WARN}
});

constexpr uint16_t buffLen = 64;
uint8_t txBuff[buffLen];
uint8_t rxBuff[buffLen] = {};

system_tick_t was = 0;

void setup() {
    Serial2.begin(115200);
    for (int i = 0; i < buffLen; i++) {
        txBuff[i] = (uint8_t)i;
    }
    was = millis();
}

bool done = true;
size_t txLen = 0;
size_t rxCnt = 0;

void loop() {
    if (done) {
        txLen = random(1, buffLen);
        txLen = Serial2.write(txBuff, txLen);
        Log.printf("TX Done: %d\r\n", txLen);
        done = false;
        was = millis();
        rxCnt = 0;
    }

    size_t len = Serial2.available();
    if (len) {
        while (len--) {
            rxBuff[rxCnt] = Serial2.read();
            Log.printf("%02d ", rxBuff[rxCnt]);
            rxCnt++;
        }
    }

    if (millis() - was > 800) {
        was = millis();
        Log.printf("Done \r\n");
        done = true;
        if (rxCnt == txLen && !memcmp(txBuff, rxBuff, txLen)) {
            Log.info("Test Passed! len: %d", rxCnt);
        } else {
            Log.info("Test Failed! TX: %d, RX: %d", txLen, rxCnt);
            Serial2.breakTx();
            while (1);
        }
    }
}

References

N/A


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@@ -310,7 +310,7 @@ inline ssize_t RingBuffer<T>::acquireCommit(size_t size, size_t cancel) {

headPending_ -= (size + cancel);
head_ = wrap(head_ + size, curSize_);
full_ = ((head_ == tail_) && size > 0);
full_ = ((head_ == tail_) && size > 0) || full_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's change this for readability:

if (size > 0) {
    full_ = (head_ == tail_);
}

We should only be updating full_ flag if we are modifying head_.

flushDmaRxFiFo(transferredToDmaFromUart);
} else {
toConsume = dmaAvailableInBuffer - alreadyCommitted;
// Try not suspending DMA unless necessary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if (hal_interrupt_is_isr()) {
// This method is called from DMA RX ISR
// Clear UART RX FIFO read counter ASAP, because UART might be still receiving data
uartInstance->RX_BYTE_CNT |= RUART_RX_BYTE_CNTER_CLEAR;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary to perform here. DMA will not be reading data out of UART FIFO if the transaction just finished. The counter will stay the same. This is not data put into UART FIFO, this is data transmitted from UART FIFO into DMA FIFO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that other interrupts with higher priority may interrupt the executtion of the UART RX DMA ISR, while the UART is receiving data. If we clear the counter later in the DMA ISR, we might lose some bytes, especially with higher baudrate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't. We are resetting RX_BYTE_CNT only at the end of the DMA transaction. Once DMA transaction finishes RX_BYTE_CNT will no longer increase because DMA no longer transfers data out of UART FIFO into its internal FIFO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see 👍 . Accordding to the REG_RX_BYTE_CNT's description:

Counting the byte number of data reading from Rx FIFO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's an actual number of bytes already transferred from UART FIFO into DMA FIFO, so already in DMA FIFO.

@XuGuohui XuGuohui merged commit 3c3b8b1 into develop Aug 15, 2022
@XuGuohui XuGuohui deleted the fix/p2_uart_dma branch August 15, 2022 07:30
@technobly technobly added the bug label Aug 15, 2022
@technobly technobly added this to the 5.0.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants